PDEState: unconditionally close stream #635
Conversation
Test Results 246 files + 6 246 suites +6 1h 11m 1s ⏱️ + 30m 40s For more details on these errors, see this check. Results for commit 617669a. ± Comparison against base commit 7dd409f. This pull request removes 1 test.♻️ This comment has been updated with latest results. |
build/org.eclipse.pde.build/src/org/eclipse/pde/internal/build/site/PDEState.java
Outdated
Show resolved
Hide resolved
| try (ZipFile jarFile = new ZipFile(bundleLocation, ZipFile.OPEN_READ)) { | ||
| ZipEntry manifestEntry = jarFile.getEntry(JarFile.MANIFEST_NAME); | ||
| if (manifestEntry != null) { | ||
| try (InputStream manifestStream = jarFile.getInputStream(manifestEntry)) { |
There was a problem hiding this comment.
"Closing this ZIP file will close all of the input streams previously returned by invocations of the getInputStream".
So I think we can skip the nested try in this case. :)
There was a problem hiding this comment.
We probably could, but it's easier to read (also for static analysis tools) to just close any stream instead of looking into javadoc.
| @@ -317,45 +317,32 @@ private String getQualifierPropery(String bundleLocation) { | |||
|
|
|||
| //Return a dictionary representing a manifest. The data may result from plugin.xml conversion | |||
| private Dictionary<String, String> basicLoadManifest(File bundleLocation) { | |||
There was a problem hiding this comment.
I think it would simplify the method if the processing of the input stream would be extraced into a separat method and then called from both locations.
Together with the other two suggestions the code would then be:
private Dictionary<String, String> basicLoadManifest(File bundleLocation) {
try {
if ("jar".equalsIgnoreCase(IPath.fromOSString(bundleLocation.getName()).getFileExtension()) && bundleLocation.isFile()) { //$NON-NLS-1$
try (ZipFile jarFile = new ZipFile(bundleLocation, ZipFile.OPEN_READ)) {
ZipEntry manifestEntry = jarFile.getEntry(JarFile.MANIFEST_NAME);
if (manifestEntry != null) {
return readManifestEntries(jarFile.getInputStream(manifestEntry));
}
}
} else {
try (InputStream manifestStream = new FileInputStream(new File(bundleLocation, JarFile.MANIFEST_NAME))) {
return readManifestEntries(manifestStream);
}
}
} catch (IOException | BundleException e) {
//ignore
}
//It is not a manifest, but a plugin or a fragment
return null;
}
private Dictionary<String, String> readManifestEntries(InputStream manifestStream) throws IOException, BundleException {
Hashtable<String, String> result = new Hashtable<>();
result.putAll(ManifestElement.parseBundleManifest(manifestStream, null));
return result;
}
There was a problem hiding this comment.
The overhead of introducing an additional method is in this case bigger then the code reduction.
For eager return on manifestStream == null jarFile was not closed.
|
ignoring random fail PlainJavaProjectTest #555 |
For eager return on manifestStream == null jarFile was not closed.